Patterns: Fix issue with template in replace template screen #56407
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
[Type] Bug
|
Size Change: +67 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
glendaviesnz
commented
Nov 22, 2023
| @@ -71,7 +72,9 @@ function BlockPattern( { | |||
| } } | |||
| > | |||
| <WithToolTip | |||
| showTooltip={ showTooltip && ! pattern.id } | |||
There was a problem hiding this comment.
The existing bug really only needed the change to pattern.type in this file, but I thought it was better to update all the uses of the pattern.id check now as the issue may appear in other areas if we start mixing patterns and templates more in the future.
There was a problem hiding this comment.
Thanks for the fix here @glendaviesnz 👍
Looks like there are a couple of places in the code that still need to be switched to user pattern.type:
That was only after an quick search so I wouldn't take that list as exhaustive. There could be others.
|
🚀 Looks like you pushed those required updates while I was typing up the review. I'll give it another review in a moment. |
Thanks. The other instances I could find seemed to be valid uses of id but let me know if you spot any more. I have updated test instructions to cover the site editor changes. |
|
Thanks, not sure how I missed that. I have fixed and updated test instructions to check duplicated pattern sync status to cover this. |
aaronrobertshaw
approved these changes
Nov 22, 2023
There was a problem hiding this comment.
Thanks for the iterations here.
I couldn't find any further uses of pattern.id or item.id to check for theme patterns. After checking out the latest commits everything is testing as advertised for me.
✅ When switching templates, the options do not have a purple highlight or pattern icon
✅ The pattern inserter displays the correct icons and highlights for user patterns
✅ Pattern inserter's sync filters work as expected
✅ Site editor's pattern management page sync filters continue to work
✅ Duplicating user and theme patterns work correctly
glendaviesnz
added
the
Backport to WP Minor Release
|
For backporting there have been quite a few changes in the affected areas since 6.4, so it might be easier to just do a small patch for the specific bug rather than this wider refactor. Ping me when the next 6.4.* build it being done and I can co-ordinate this. |
👋 @glendaviesnz, 6.4.2 is currently being worked on. We have a PR here for the package updates: WordPress/wordpress-develop#5698. It hasn't been committed yet, so this change could still be included. I'm at a meetup next week so may be slow to reply, but it may be worth coordinating the patch for this change next week. cc. @karmatosed @SiobhyB @tellthemachines |
I'm happy to help where needed! Is there a date set for 6.4.2 yet? |
|
@tellthemachines, @SiobhyB, @karmatosed I think it is probably best to only include the bug fix changes in 6.4.2 rather than the full refactor in this PR, eg. just the changes in this file. Is the best way to do that to open a PR with just that change against wp/6.4 branch? |
I'd say so, yes. Definitely let's avoid adding new features in a minor release 😅 |
|
Have added a minimal fix here #56538 |
tellthemachines
removed
the
Backport to WP Minor Release
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes an issue with some templates displaying as synced patterns by mistake.
Why?
In the switch templates screen on the page editor the templates show the synced pattern icon and highlighting which is confusing to users.
Fixes: #56380
How?
This screen was identifying user patterns by the presence of
pattern.idbut it turns out that some templates also have anidfield set. This PR adds atype=userfield to user patterns to more reliably identify them.Testing Instructions
Userstill work as expectedScreenshots or screencast
Before:
pattern-icon-before.mp4
After:
pattern-icon-after.mp4